Adding onCancel and onClose callbacks #16904
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThe changes add a new optional Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/sdk/src/browser/index.tsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/sdk/CHANGELOG.md(2 hunks)packages/sdk/package.json(1 hunks)packages/sdk/src/browser/index.ts(3 hunks)
🧰 Additional context used
🪛 GitHub Check: test
packages/sdk/CHANGELOG.md
[failure] 9-9: Line length
packages/sdk/CHANGELOG.md:9:81 MD013/line-length Line length [Expected: 80; Actual: 122] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md013.md
[failure] 10-10: Line length
packages/sdk/CHANGELOG.md:10:81 MD013/line-length Line length [Expected: 80; Actual: 131] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md013.md
🪛 GitHub Actions: Lint SDK Markdown Files
packages/sdk/CHANGELOG.md
[error] 9-9: markdownlint MD013/line-length: Line length exceeds 80 characters (Actual: 122). See https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md013.md
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
🔇 Additional comments (5)
packages/sdk/package.json (1)
4-4: LGTM! Appropriate version bump for new functionality.The version increment from 1.6.3 to 1.6.4 correctly reflects the addition of new optional callback features without breaking existing functionality.
packages/sdk/CHANGELOG.md (1)
71-72: Good formatting improvement.The formatting fix improves readability of the PD_SDK_DEBUG environment variable description.
packages/sdk/src/browser/index.ts (3)
113-123: Well-designed callback interface with clear documentation.The new optional callbacks are properly typed with comprehensive JSDoc comments that clearly explain their behavior and when they're invoked. The optional nature maintains backwards compatibility.
247-247: Excellent implementation logic for tracking connection state.The
connectionSuccessfulflag correctly tracks the connection lifecycle:
- Initialized as
falsefor each connection attempt- Set to
trueonly on successful connection- Used to determine whether to call
onCancelwhen the iFrame closesThis implementation ensures
onClosefires on any closure whileonCancelonly fires when the user cancels without successfully connecting.Also applies to: 252-252, 262-266
237-242: Comprehensive example documentation for new callbacks.The JSDoc example effectively demonstrates how to use the new
onCloseandonCancelcallbacks with clear, practical usage patterns that align with the existing documentation style.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/sdk/CHANGELOG.md (1)
9-10: Break long lines and add missing article to meet markdown linting.The description exceeds 80 characters (MD013) and is missing the article “the” before
ConnectStatus. Please split the content into shorter lines under 80 characters and insert “the” for clarity:- Added `onClose` callback to `connectAccount` method that receives a - `ConnectStatus` object with `successful` and `completed` boolean properties + Added `onClose` callback to the `connectAccount` method. + It receives the `ConnectStatus` object with `successful` and + `completed` boolean properties.🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: You might be missing the article “the” here.
Context: ...## Added - AddedonClosecallback toconnectAccountmethod that receives a ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/sdk/CHANGELOG.md(2 hunks)packages/sdk/src/browser/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk/src/browser/index.ts
🧰 Additional context used
🪛 LanguageTool
packages/sdk/CHANGELOG.md
[uncategorized] ~9-~9: You might be missing the article “the” here.
Context: ...## Added - Added onClose callback to connectAccount method that receives a ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Lint Code Base
🔇 Additional comments (2)
packages/sdk/CHANGELOG.md (2)
5-6: Version header update is correct.
71-72: PD_SDK_DEBUG description formatting adjustment is appropriate.
2 participants
WHY
Summary by CodeRabbit
New Features
Documentation
Chores